Call completeStream only once on failure. #13814

gregw · 2025-10-21T00:07:11Z

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback.

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

gregw · 2025-10-21T01:45:16Z

The issue is fundamentally that completeStream is being called twice.

For this to happen, we need a failure to be detected in the ChannelCallback.succeeded() method so that the following code isrun:

jetty.project/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

Lines 1573 to 1580 in 8dbd986

    
           if (failure != null) 
        
           { 
        
               httpChannelState._callbackFailure = failure; 
        
               if (!stream.isCommitted()) 
        
                   errorResponse = new ErrorResponse(request); 
        
               else 
        
                   completeStream = true; 
        
           }

This means that completeStream will be called, even though the other "legs of the 3 legged stool" are not complete - specifically we may still be inside the call to HandlerInvoker.run(), as in this stack for thread 258 :

2025-10-20 08:14:42,457 INFO  [WebServerImpl-258] trace.jetty.session.complete: complete() called on session [ManagedSession@4df349fb{id=MYSECRETSESSIONID,x=MYSECRETSESSIONID.node0,req=6,res=true}]
java.lang.Exception: complete stack
	...
	at org.eclipse.jetty.session.AbstractSessionManager.complete(AbstractSessionManager.java)
	at org.eclipse.jetty.session.AbstractSessionManager$SessionStreamWrapper.doComplete(AbstractSessionManager.java:1509)
	at org.eclipse.jetty.server.handler.ContextHandler$ScopedContext.run(ContextHandler.java:1518)
	at org.eclipse.jetty.session.AbstractSessionManager$SessionStreamWrapper.failed(AbstractSessionManager.java:1479)
	at org.eclipse.jetty.server.internal.HttpChannelState$HandlerInvoker.completeStream(HttpChannelState.java:788)
	at org.eclipse.jetty.server.internal.HttpChannelState$ChannelCallback.succeeded(HttpChannelState.java:1591)
	at org.eclipse.jetty.server.handler.gzip.GzipResponseAndCallback.succeeded(GzipResponseAndCallback.java:95)
	at org.eclipse.jetty.ee10.servlet.ServletChannel.onCompleted(ServletChannel.java:765)
	at org.eclipse.jetty.ee10.servlet.ServletChannel.handle(ServletChannel.java:429)
	at org.eclipse.jetty.ee10.servlet.ServletHandler.handle(ServletHandler.java:470)
	at org.eclipse.jetty.ee10.servlet.SessionHandler.handle(SessionHandler.java:717)
	at org.eclipse.jetty.server.handler.ContextHandler.handle(ContextHandler.java:1071)
	at org.eclipse.jetty.server.Handler$Wrapper.handle(Handler.java:740)
	at org.eclipse.jetty.rewrite.handler.RewriteHandler.handle(RewriteHandler.java:138)
	at org.eclipse.jetty.server.handler.gzip.GzipHandler.handle(GzipHandler.java:611)
	at org.eclipse.jetty.server.Handler$Sequence.handle(Handler.java:805)
	at org.eclipse.jetty.server.Server.handle(Server.java:182)
	at org.eclipse.jetty.server.internal.HttpChannelState$HandlerInvoker.run(HttpChannelState.java:677)
	at org.eclipse.jetty.util.thread.Invocable$ReadyTask.run(Invocable.java:177)
	at org.eclipse.jetty.http2.server.internal.HttpStreamOverHTTP2$1.run(HttpStreamOverHTTP2.java:144)
	...

We can see that there is a failure detected in ChannelCallback.succeeded() because SessionStreamWrapper.failed is ultimately called. This means that there must have been one of the following application errors:

jetty.project/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

Line 1543 in 8dbd986

failure = ExceptionUtil.combine(failure, new IllegalStateException("demand pending"));
jetty.project/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

Line 1545 in 8dbd986

failure = ExceptionUtil.combine(failure, new IllegalStateException("write pending"));
jetty.project/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

Line 1563 in 8dbd986

failure = ExceptionUtil.combine(failure, new IOException("content-length %d != %d written".formatted(committedContentLength, totalWritten)));

These are all plausible application errors, especially with something like server sent events.

So once thread 258 has called completeStream it returns all the way out of handling and it can be seen calling completeStream again, in this stack trace:

java.lang.Exception: complete stack
	...
	at org.eclipse.jetty.session.AbstractSessionManager.complete(AbstractSessionManager.java)
	at org.eclipse.jetty.session.AbstractSessionManager$SessionStreamWrapper.doComplete(AbstractSessionManager.java:1509)
	at org.eclipse.jetty.server.handler.ContextHandler$ScopedContext.run(ContextHandler.java:1524)
	at org.eclipse.jetty.session.AbstractSessionManager$SessionStreamWrapper.failed(AbstractSessionManager.java:1479)
	at org.eclipse.jetty.server.internal.HttpChannelState$HandlerInvoker.completeStream(HttpChannelState.java:788)
	at org.eclipse.jetty.server.internal.HttpChannelState$HandlerInvoker.run(HttpChannelState.java:712)
	at org.eclipse.jetty.util.thread.Invocable$ReadyTask.run(Invocable.java:177)
	at org.eclipse.jetty.http2.server.internal.HttpStreamOverHTTP2$1.run(HttpStreamOverHTTP2.java:144)
	...

which is called from this code after the handler has been invoked:

jetty.project/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

Lines 691 to 713 in 8dbd986

    
           try (AutoLock ignored = _lock.lock()) 
        
           { 
        
               stream = _stream; 
        
               _handling = null; 
        
               _handled = true; 
        
               failure = _callbackFailure; 
        
               callbackCompleted = _callbackCompleted; 
        
               lastStreamSendComplete = lockedIsLastStreamSendCompleted(); 
        
               completeStream = callbackCompleted && lastStreamSendComplete; 
        
               if (LOG.isDebugEnabled()) 
        
                   LOG.debug("handler invoked: completeStream={} failure={} callbackCompleted={} {}", completeStream, failure, callbackCompleted, HttpChannelState.this); 
        
           } 
        
           if (LOG.isDebugEnabled()) 
        
               LOG.debug("stream={}, failure={}, callbackCompleted={}, completeStream={}", stream, failure, callbackCompleted, completeStream); 
        
           if (completeStream) 
        
           { 
        
               if (LOG.isDebugEnabled()) 
        
                   LOG.debug("completeStream({}, {})", stream, Objects.toString(failure)); 
        
               completeStream(stream, failure); 
        
           }

Note that in order for this code to actually call completeStream then it must be true that completeStream = callbackCompleted && lastStreamSendComplete. Note that this is normally not the case for HTTP/1 because the first call to completeStream would have recycled the HttpChannelState and the callbackCompleted and lastStreamSendComplete will both be false. However, for H2, HttpChannelStates are re-used after being recycled, so another request may have come in and set the fields of the state again, so that the second call to completeStream incorrectly completes that new request.

Thus I believe the core fix is to not call completeStream whilst we are still handling. Furthermore, if we are to ignore the last write leg of the stool, we should explicitly force lastStreamSendComplete to true;

Unfortunately I have been unable to produce a unit test for this, as I believe it needs precisely unlucky timing and an application error.

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback.

gregw · 2025-10-21T01:54:55Z

@sbordet @lorban Can you review the diagnosis that @janbartel and I have come up with. I'm 90% sure this is it, but I cannot reproduce (any thoughts how we might be able to do that?).

gregw · 2025-10-21T02:00:48Z

Note that we added this completeStream call in #9684

sbordet

ChannelCallback.failed() seems not entirely correct either.

Can we write test cases for this scenario?

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

sbordet · 2025-10-21T20:24:39Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

+                        // We are committed and still handling, so let the HandlerInvoker complete, ignoring any pending reads/writes.
+                        httpChannelState._streamSendState = StreamSendState.LAST_COMPLETE;


Again, not sure we should ignore pending writes.

Or pending reads.... it is a difficult one. I'll at least change the code to enumerate the possible states

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

gregw · 2025-10-21T20:44:31Z

ChannelCallback.failed() seems not entirely correct either.

@sbordet I will look....

Can we write test cases for this scenario?

Very hard, because unless there is another thread racing the second completeStream is not called. I'm open to suggestions.

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method.

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method. Improved EventSourceServlet

gregw · 2025-10-22T09:56:56Z

@sbordet @lorban I'm getting concerned at the number of tests this PR is breaking in its current state.
I think we need to take time to consider the more "cleanup" changes and probably only make this in 12.1.x

So I propose that this PR should simply be 83c1718 for 12.0.x (perhaps with the EventSourceServlet cleanups), and then we can do a wider cleanup and refactor in 12.1.x next month.

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method. Improved EventSourceServlet

lorban · 2025-10-23T12:31:22Z

Regarding the minimal 12.0 fix, I think httpChannelState._handling == null should actually be httpChannelState._handled

gregw · 2025-10-24T01:54:46Z

@lorban this is passing tests now.... so let's go for this one?
@sbordet @lorban Review please!!

sbordet · 2025-10-24T09:46:49Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

+                    Throwable unconsumed = stream.consumeAvailable();
+                    if (failure != null)
+                        ExceptionUtil.addSuppressedIfNotAssociated(failure, unconsumed);


Just ExceptionUtil.combine(failure, stream.consumeAvailable()); as the line above?
But then, this else block is identical to the else-if above?

No this is different. If failure==null, then it remains null. i.e. it is not a failure to not consume. I'll comment....

Actually, I will try without the whole branch....

Branch is needed

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/ExceptionUtil.java

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

sbordet · 2025-10-24T12:20:16Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

-    private class HandlerInvoker implements Invocable.Task, Callback
+    // HandlerInvoker is used as the Response's _writeCallback when ChannelCallback is succeeded and the last send still
+    // needs to be done, i.e.: _streamSendState set to LAST_SENDING by lockedLastStreamSend().
+    private class HandlerInvoker implements Task, Callback


I think we should split the functionality of this class.
Leave run() in HandlerInvoker, but move the Callback functionality into a LastStreamSendCallback class.

This is really not a 12.0 thing then

sbordet · 2025-10-24T12:42:45Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

+                            failedCallback = response._writeCallback;
+                            response._writeCallback = httpChannelState._handlerInvoker;


In this way, we may wait forever for the write to complete and invoke the _handlerInvoker.

How about this:

Suggested change

failedCallback = response._writeCallback;

response._writeCallback = httpChannelState._handlerInvoker;

Runnable task = response.lockedFailWrite(failure);

failedCallback = Callback.from(task, httpChannelState._handlerInvoker);

In 12.1.x we will leverage the new cancelSend() feature automatically.

I'm not sure that continuing on with a pending write pointing at the HttpChannelState is a good idea. Feels like an invitation for a race. What is wrong waiting for the write to complete. It won't be forever unless they have disabled idle timeout.

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

sbordet · 2025-10-24T13:00:10Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

            {
                assert _callbackCompleted;
                _streamSendState = StreamSendState.LAST_COMPLETE;
                completeStream = _handling == null;


Suggested change

completeStream = _handled;

I tried, but we also need to complete a stream in cases where we have not started handling yet.

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

…470-completeStreamOnce

gregw closed this Oct 21, 2025

gregw force-pushed the fix/12.0.x/13470-completeStreamOnce branch from a8cf31d to bee65a6 Compare October 21, 2025 00:15

gregw mentioned this pull request Oct 21, 2025

Jetty 12.0: ManagedSession issues due to recursion and/pr multiple completions of the stream. #13470

Open

Call completeStream only once on failure.

daa9a47

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback.

gregw reopened this Oct 21, 2025

gregw linked an issue Oct 21, 2025 that may be closed by this pull request

Jetty 12.0: ManagedSession issues due to recursion and/pr multiple completions of the stream. #13470

Open

gregw commented Oct 21, 2025

View reviewed changes

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java Outdated Show resolved Hide resolved

Call completeStream only once on failure.

83c1718

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback.

gregw added the save-home label Oct 21, 2025

gregw added this to Jetty 12.0.30 Oct 21, 2025

gregw requested review from janbartel, lorban and sbordet October 21, 2025 20:08

sbordet requested changes Oct 21, 2025

View reviewed changes

gregw added 3 commits October 22, 2025 11:02

Call completeStream only once on failure.

2973561

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method.

Call completeStream only once on failure.

2f7aa04

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method. Improved EventSourceServlet

Call completeStream only once on failure.

bcbf2c8

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method. Improved EventSourceServlet

gregw requested a review from sbordet October 22, 2025 19:50

gregw added 3 commits October 23, 2025 07:33

Call completeStream only once on failure.

42e209f

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method. Improved EventSourceServlet

Call completeStream only once on failure.

baca52e

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method. Improved EventSourceServlet

Call completeStream only once on failure.

a326415

Fix #13470 by calling completeStream only once even when there is a failure in the channel callback. refactor success and failure into single method. Improved EventSourceServlet

sbordet requested changes Oct 24, 2025

View reviewed changes

updates from review

6bea600

gregw requested a review from sbordet October 24, 2025 21:04

Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/13…

649e48b

…470-completeStreamOnce

		// We are committed and still handling, so let the HandlerInvoker complete, ignoring any pending reads/writes.
		httpChannelState._streamSendState = StreamSendState.LAST_COMPLETE;

		failedCallback = response._writeCallback;
		response._writeCallback = httpChannelState._handlerInvoker;

Call completeStream only once on failure. #13814

Are you sure you want to change the base?

Call completeStream only once on failure. #13814

Conversation

gregw commented Oct 21, 2025

Uh oh!

Uh oh!

gregw commented Oct 21, 2025

Uh oh!

gregw commented Oct 21, 2025

Uh oh!

gregw commented Oct 21, 2025

Uh oh!

sbordet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gregw commented Oct 21, 2025

Uh oh!

gregw commented Oct 22, 2025

Uh oh!

lorban commented Oct 23, 2025

Uh oh!

gregw commented Oct 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants